Decouple the Clipboard API from fyne.Window #5121

pierrec · 2024-09-08T08:20:36Z

Description:

This PR implements the changes for #4418 by:

updating the fyne.App, fyne.Driver and fyne.Window interfaces
migrating tests and cmd programs using the clipboard to the new interface

Fixes #4418

Checklist:

Tests included.
I dont see any automated tests for this but maybe I missed them. I did test manually and ctrl-c, ctrl-v and ctrl-x work as expected on the desktop.
Lint and formatter run with no errors.
Tests all pass.
I get quite a few errors on my system, some related to the locale (labels are emitted in French while tests expect English), others to something else which I dont know about.
They all seem unrelated to this change though.

Where applicable:

Public APIs match existing style and have Since: line.
Any breaking changes have a deprecation path or have been discussed.
Check for binary size increases when importing new modules.
(no new module imported)

Jacalz

Nice work. I changed the base branch to develop and that made the changes look a lot more sensible. Just added a small comment about that we perhaps should consider changing a function signature (if you agree with me there) :)

cmd/fyne_demo/main.go

driver.go

internal/driver/glfw/window.go

Jacalz · 2024-09-09T17:38:54Z

@pierrec Please don't force push your changes very time. Descriptive commits with are lot better as we more easily can see what has changed and GitHub also has a tendency to sometimes lose track of old review comments. We'll squash on merge if needed.

pierrec · 2024-09-09T17:49:15Z

@pierrec Please don't force push your changes very time. Descriptive commits with are lot better as we more easily can see what has changed and GitHub also has a tendency to sometimes lose track of old review comments. We'll squash on merge if needed.

OK, so sorry, will just push next time.

pierrec · 2024-09-09T19:47:48Z

Question while working through the CI failures (a lot of them due to my lack of knowledge of the Fyne codebase and all the build flags!):
what to do with the TestWindow_Clipboard and TestWindow_ClipboardCopy_DisabledEntry tests in internal/driver/glfw/window_test.go? Should they be moved to app/app_test.go?

internal/driver/mobile/driver.go

internal/driver/glfw/driver.go

app/app_test.go

andydotxyz · 2024-09-13T16:33:51Z

I think there may have been a confusion about word usage here - so I want to ensure we are on the same page.

Rather than allocating a new clipboard, should we just return the singleton instance from the app here?

I think that @dweymouth was suggesting that the clipboard exist inside the app instance instead of inside the window where it was before. That is neither a true singleton, but exists only once per application. It will be much more efficient than the current implementation which seems to allocate a new clipboard for every situation where it may be used.

Jacalz · 2024-09-13T23:07:44Z

It will be much more efficient than the current implementation which seems to allocate a new clipboard for every situation where it may be used.

I'm really not sure that it is a correct assumption. All of our clipboard structs are zero size as I can remember and don't actually contain any information so allocating extra size for the interface is unnecessary? We can probably just avoid returning a pointer to the struct and help it not allocate that way.

I don't think it is worth making the app struct larger just for an arbitrary optimisation that we don't if it is worth it or not. Getting the clipboard is likely not a hot path in any application and if anyone cares, they can just save it to a variable themselves and be happy there.

coveralls · 2024-09-14T17:54:18Z

coverage: 66.037% (-0.02%) from 66.058%
when pulling a26d502 on pierrec:issue4418
into 6ea393e on fyne-io:develop.

Jacalz · 2024-09-16T19:56:42Z

Sorry for the long turnaround time for me reviewing this. I've had quite a busy first weeks with the first weeks of my masters program. I'll try to have a look one of the coming days.

pierrec · 2024-09-17T17:04:49Z

Sorry for the long turnaround time for me reviewing this. I've had quite a busy first weeks with the first weeks of my masters program. I'll try to have a look one of the coming days.

Thank you for the update, there is no rush :).

Jacalz

Sorry for the very long wait. Just one minor change needed and we're golden :)

Jacalz · 2024-09-25T08:58:42Z

internal/driver/mobile/driver.go

@@ -100,6 +100,10 @@ func (d *driver) currentWindow() *window {
 	return last
 }

+func (d *driver) Clipboard() fyne.Clipboard {
+	return &mobileClipboard{}


I think this should return a value and not a pointer for consistency? Maybe even use the NewClipboard() function?

This one slipped through. Will do.

… instance

Jacalz · 2024-09-25T10:39:14Z

The static analysis test failure seemed a bit strange. Kicked it to see if it is temporary.

pierrec · 2024-09-25T11:21:16Z

The static analysis test failure seemed a bit strange. Kicked it to see if it is temporary.

I had a look but I dont even have that statement on line 406 of the file; grepping doesnt return any instance of it either.

Ad running it locally returns errors but not that one:

app/icon_cache_file.go:11:5: var once is unused (U1000)
app/icon_cache_file.go:13:19: func (*fyneApp).cachedIconPath is unused (U1000)
app/icon_cache_file.go:30:6: func rootCacheDir is unused (U1000)
app/icon_cache_file.go:35:19: func (*fyneApp).saveIconToCache is unused (U1000)
cmd/fyne/internal/mobile/binres/binres_test.go:249:10: printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
internal/driver/glfw/driver_desktop.go:25:7: const desktopDefaultDoubleTapDelay is unused (U1000)
internal/driver/mobile/app/app.go:164:15: func (*app).registerGLViewportFilter is unused (U1000)
widget/check_group.go:185:5: should omit nil check; len() for []*fyne.io/fyne/v2/widget.Check is defined as zero (S1009)
widget/list.go:172:5: should omit nil check; len() for map[fyne.io/fyne/v2/widget.ListItemID]float32 is defined as zero (S1009)
widget/list.go:371:5: should omit nil check; len() for map[fyne.io/fyne/v2/widget.ListItemID]float32 is defined as zero (S1009)
widget/radio_group.go:159:5: should omit nil check; len() for []fyne.io/fyne/v2.CanvasObject is defined as zero (S1009)

Add NewSelectWithData function. Add Select.Bind and Select.Unbind methods to bind a string to Select.Selected.

Test that binding actually occurred.

Currently translated at 100.0% (39 of 39 strings) Translation: Fyne/Fyne Translate-URL: https://hosted.weblate.org/projects/fyne/fyne/sv/

… error Fixes fyne-io#3353.

This mainly does two things: - Uses Go 1.22 instead of 1.21 as the highest version (I don't have 1.23 on Fedora 40 yet so did not want to bump to latest just yet). - We now always pull the latest setup-go-faster action in v1 without having to manually change version.

This fixes a few code errors that slipped through review (thankfully now handled by Staticcheck for us) and makes sure we are using the latest version.

Haven't quite gotten used to Nvim yet ;)

This should be safer than ever given that some new features are dependent on the version in go.mod.

pierrec · 2024-10-01T13:15:32Z

Still erroring out. At this point I might close this PR and make another one.

Jacalz · 2024-10-01T18:22:46Z

Let me just check out your branch locally to see if there is anything strange going on there before you do anything :)

Jacalz · 2024-10-01T18:32:52Z

It does indeed still error out but the line it points to happens to be a perfect valid error that easily can be fixed.

Jacalz

Thanks for working on this. I'm really sorry for the long turnaround time and all the problems that popped up. Hopefully it will be smooth sailing for any future contributions :)

pierrec · 2024-10-01T19:53:25Z

Thanks for the review!

andydotxyz

Questioning some removed code.

The test failure looks like a reduction in code coverage - It isn't an obvious lack in this PR but I wonder if somehow it has redirected code to be less well tested. The Coveralls report should lend a hint.

andydotxyz · 2024-10-01T20:41:17Z

cmd/fyne_demo/main.go

-func shortcutFocused(s fyne.Shortcut, w fyne.Window) {
-	switch sh := s.(type) {
-	case *fyne.ShortcutCopy:
-		sh.Clipboard = w.Clipboard()


Removing this code looks like it is possible that the shortcut for copy.paste etc could some times be missing the clipboard info which as previously there..?

From what I can tell from the code, it was always setting the clipboard twice. First when creating the struct and then this function made a type switch and set it again?

@andydotxyz the shortcut is already setup with the clipboard at instanciation. No need to do it a second time right?

the shortcut is already setup with the clipboard at instanciation.

In the instantiation you make, yes - but check other usages too - like in Entry...

e.shortcut.AddShortcut(&fyne.ShortcutCopy{}, ...)

I see. It may be better to get ShortcutCopy/Cut/Paste from the App though instead of relying on the user to properly instantiate it. In the meantime, I will restore the check in the fyne_demo app.

It may be better to get ShortcutCopy/Cut/Paste from the App though instead of relying on the user to properly instantiate it

I agree with this statement, however having the fields be nil would be a breaking change, so thanks for re-instating it.

andydotxyz · 2024-10-03T13:12:52Z

Excellent thanks so much, I think we just need the approval of changes from @dweymouth now too. The coverage issue persists but without it going to coveralls I cannot see what the issue is. I'm happy that you have added tests so I can merge this after changes are approved and we can continue to investigate the 63% coverage failure.

Jacalz changed the base branch from master to develop September 8, 2024 10:34

Jacalz reviewed Sep 8, 2024

View reviewed changes

cmd/fyne_demo/main.go Outdated Show resolved Hide resolved

pierrec force-pushed the issue4418 branch from ebdfb61 to 55c6d11 Compare September 8, 2024 11:43

dweymouth requested changes Sep 8, 2024

View reviewed changes

driver.go Outdated Show resolved Hide resolved

dweymouth reviewed Sep 9, 2024

View reviewed changes

internal/driver/glfw/window.go Show resolved Hide resolved

dweymouth changed the title ~~Issue4418~~ Decouple the Clipboard API from fyne.Window Sep 9, 2024

Add fyne.App.Clipboard method, fixes fyne-io#4418

83427a8

pierrec force-pushed the issue4418 branch from f4cbbdd to 83427a8 Compare September 9, 2024 15:34

pierre added 2 commits September 10, 2024 08:40

add missing Clipboard for various platforms; update tests

2999a08

test: return proper Clipboard

dd612d7

dweymouth reviewed Sep 11, 2024

View reviewed changes

internal/driver/mobile/driver.go Outdated Show resolved Hide resolved

internal/driver/glfw/driver.go Outdated Show resolved Hide resolved

pierre added 3 commits September 12, 2024 18:44

Clipboard: remove singleton

3482cae

app: add missing NewClipboard()

992f113

app: instantiate a window to initialize glfw for clipboard tests

616fff2

Jacalz reviewed Sep 12, 2024

View reviewed changes

app/app_test.go Outdated Show resolved Hide resolved

app: Clipboard: use test.NewTempApp and NewTempWindow

d6a142f

pierre added 2 commits September 14, 2024 10:15

app: remove duplicat import decl

395444b

Clipboard: do not return pointer for a clipboard instance

a26d502

Jacalz requested changes Sep 25, 2024

View reviewed changes

internal/driver/mobile: use NewClipboard() instead of returning a new…

6cdb472

… instance

jimorc and others added 16 commits October 1, 2024 15:07

Add data binding to Select widget

8d2017e

Add NewSelectWithData function. Add Select.Bind and Select.Unbind methods to bind a string to Select.Selected.

Update TeastNewSelectWithData

54e11f6

Test that binding actually occurred.

Add Since line to public APIs

7eb3402

Change Since to 2.6 from 2.0

fff9c45

Change Since 2.6 to Since: 2.6

453d3cb

Added translation using Weblate (Swedish)

e30975a

Translated using Weblate (Swedish)

7738644

Currently translated at 100.0% (39 of 39 strings) Translation: Fyne/Fyne Translate-URL: https://hosted.weblate.org/projects/fyne/fyne/sv/

internal/driver/mobile: return storage.ErrNotExists instead of custom…

7b6f833

… error Fixes fyne-io#3353.

Update root package GoDoc to include internal links

219e798

Improve doc strings for file dialogs

b764796

Further improve comments for file dialogs

54a2d88

Update to the latest breaking API changes in go-text

70984a8

Update CI and code to use Staticcheck v0.5.1

7ba5aa5

This fixes a few code errors that slipped through review (thankfully now handled by Staticcheck for us) and makes sure we are using the latest version.

Add a missing import

4bfe10d

Haven't quite gotten used to Nvim yet ;)

Bump Go version in CI to 1.23

5e52896

This should be safer than ever given that some new features are dependent on the version in go.mod.

Jacalz added 2 commits October 1, 2024 20:33

Fix staticcheck error for deprecation

c2cdd54

Merge remote-tracking branch 'upstream/develop' into issue4418

70576c8

Jacalz previously approved these changes Oct 1, 2024

View reviewed changes

andydotxyz requested changes Oct 1, 2024

View reviewed changes

cmd/fyne_demo: set the clipboard for Copy/Cut/Paste shortcuts

50b23bb

pierrec dismissed Jacalz’s stale review via 50b23bb October 2, 2024 13:43

andydotxyz approved these changes Oct 3, 2024

View reviewed changes

dweymouth approved these changes Oct 4, 2024

View reviewed changes

andydotxyz merged commit 4fab19a into fyne-io:develop Oct 4, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple the Clipboard API from fyne.Window #5121

Decouple the Clipboard API from fyne.Window #5121

pierrec commented Sep 8, 2024

Jacalz left a comment

Jacalz commented Sep 9, 2024

pierrec commented Sep 9, 2024

pierrec commented Sep 9, 2024

andydotxyz commented Sep 13, 2024

Jacalz commented Sep 13, 2024

coveralls commented Sep 14, 2024

Jacalz commented Sep 16, 2024

pierrec commented Sep 17, 2024

Jacalz left a comment

Jacalz Sep 25, 2024

pierrec Sep 25, 2024

Jacalz commented Sep 25, 2024

pierrec commented Sep 25, 2024 •

edited

Loading

pierrec commented Oct 1, 2024

Jacalz commented Oct 1, 2024

Jacalz commented Oct 1, 2024

Jacalz left a comment

pierrec commented Oct 1, 2024

andydotxyz left a comment

andydotxyz Oct 1, 2024

Jacalz Oct 2, 2024

pierrec Oct 2, 2024

andydotxyz Oct 2, 2024

pierrec Oct 2, 2024

andydotxyz Oct 3, 2024

andydotxyz commented Oct 3, 2024

Decouple the Clipboard API from fyne.Window #5121

Decouple the Clipboard API from fyne.Window #5121

Conversation

pierrec commented Sep 8, 2024

Description:

Checklist:

Where applicable:

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz commented Sep 9, 2024

pierrec commented Sep 9, 2024

pierrec commented Sep 9, 2024

andydotxyz commented Sep 13, 2024

Jacalz commented Sep 13, 2024

coveralls commented Sep 14, 2024

Jacalz commented Sep 16, 2024

pierrec commented Sep 17, 2024

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz commented Sep 25, 2024

pierrec commented Sep 25, 2024 • edited Loading

pierrec commented Oct 1, 2024

Jacalz commented Oct 1, 2024

Jacalz commented Oct 1, 2024

Jacalz left a comment

Choose a reason for hiding this comment

pierrec commented Oct 1, 2024

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Oct 3, 2024

pierrec commented Sep 25, 2024 •

edited

Loading